fix(ghost): use well-known redirect port when scheme is unchanged - #80
Merged
Conversation
build_location only substituted the redirect scheme's well-known port when
the filter scheme differed from the request scheme. The Gateway API spec for
HTTPRequestRedirectFilter.port assigns the well-known port for any non-empty
redirect scheme, regardless of whether it matches the request.
On a listener bound to a non-standard port this produced the wrong Location:
requestRedirect {scheme: http} with no port on an http-8080 listener emitted
http://example.org:8080/ where the spec wants http://example.org/.
Also fall back to the listener port for a redirect scheme with no well-known
port, instead of defaulting it to 80.
The conformance suite cannot catch this. Its 8080-listener block covers only
scheme-nil-and-port-nil, scheme-nil-and-port-80, and scheme-https-and-port-nil;
scheme-http-and-port-nil appears only in the 443 block, where the scheme does
change. On the 80 listener the same-scheme case passed by accident because the
original port was already 80. Added a unit test for the uncovered case.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #80 +/- ##
==========================================
+ Coverage 71.84% 71.94% +0.10%
==========================================
Files 41 41
Lines 6716 6716
==========================================
+ Hits 4825 4832 +7
+ Misses 1550 1543 -7
Partials 341 341 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
KealanAU
added a commit
to KealanAU/gateway
that referenced
this pull request
Jul 28, 2026
The merge of varnish#80 brought test_build_location_same_scheme_uses_well_known_port, whose no-scheme case asserts exactly what test_build_location_keeps_listener_port did; its other sub-cases repeated test_build_location_basic and test_build_location_default_ports. The vtc keeps http-8080 and https-8443 as the end-to-end proof that scheme and port derive from the socket name; the default-port-omission and portless-socket clients re-ran pure functions already unit-tested (test_should_omit_port, test_listener_port).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
build_locationinghost/src/redirect_backend.rsonly substituted the redirect scheme's well-known port when the filter scheme differed from the request scheme:The Gateway API spec for
HTTPRequestRedirectFilter.porthas no such condition:So on a listener bound to a non-standard port,
requestRedirect: {scheme: http}with noportemittedhttp://example.org:8080/where the spec wantshttp://example.org/.Fix
Port resolution is now a direct reading of the spec — explicit port wins, otherwise the redirect scheme's well-known port, otherwise the listener port:
Two behaviour changes:
else { 80 }branch assigned port 80 to any non-httpsscheme; the spec says the listener port SHOULD be used. Not reachable today since the operator only accepts http/https, but the match arms make the rule explicit rather than incidental.No change when the scheme genuinely differs — that path was already correct.
Why conformance did not catch this
HTTPRouteRedirectPortAndSchemenever exercises the buggy branch:same-namespace-with-http-listener-on-8080block has onlyscheme-nil-and-port-nil,scheme-nil-and-port-80, andscheme-https-and-port-nil.scheme-http-and-port-nilappears only in the 443 block, where the scheme does change.Added
test_build_location_same_scheme_uses_well_known_portcovering http-on-8080, https-on-8443, and thescheme: nilcontrol case that must keep 8080. Its comment records why the test exists, since conformance won't flag a regression.Testing
cargo test --lib— 78 passed.cargo clippy --libclean.Note:
cargo fmtwants to reformatbuild.rs,config.rs,director.rs, andexternal_backend.rs— pre-existing churn onmain, deliberately left out of this PR.🤖 Generated with Claude Code